C#: nullness related extractor refactorings#642
Conversation
Refactored to make it clear when `@operator.Symbol as IMethodSymbol` can be `null`.
|
This pull request introduces 7 alerts when merging 3a78fe6 into 2a30dee - view on LGTM.com new alerts:
Comment posted by LGTM.com |
hvitved
force-pushed
the
csharp/extractor/nullness-refactorings
branch
2 times, most recently
from
5bf257a to
69ca2c0
Compare
Factor `ClauseCall` out into three classes to make it clear when the fields `operand` and `declaration` can be `null`.
hvitved
force-pushed
the
csharp/extractor/nullness-refactorings
branch
from
69ca2c0 to
e808376
Compare
calumgrant
left a comment
calumgrant
left a comment
There was a problem hiding this comment.
Overall a good improvement. We really should represent LINQ clauses more explicitly in the database.
| var method = @operator.Symbol as IMethodSymbol; | ||
|
|
||
| if (GetCallType(cx, node) == CallType.Dynamic) | ||
| var callType = GetCallType(cx, node); |
There was a problem hiding this comment.
I'm actually kind-of surprised that the nullness queries don't consider method to be potentially null in this method anyway due to the as on line 120.
There was a problem hiding this comment.
It would be good practise to check is null after an as. Although if this were in the nullness queries, it could get quite noisy I guess.
There was a problem hiding this comment.
So therefore, I think a better refactoring for this code would be
if (!(method is null))
{
if (GetCallType(cx, node) == ...
cx.Emit(Tuples.expr_call(this, Method.Create(cx, method)));
}
There was a problem hiding this comment.
I agree that considering as expressions as maybe null would make sense for the maybe-null query; I will update it separately.
3 participants
These were all found by the updated nullness queries.